fixup 2202 - #2206
Merged
Merged
Conversation
<!-- agent --> RootModule.update could swallow setup failures with keep_going and then iterate sms before assignment. Start with an empty submodule list so the handled failure safely skips updates. Commit iteration could likewise read stream before assignment for processes without stdout or unsupported inputs. Raise explicit input errors instead. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixup follow-up to #2205 to address missed commits by hardening a few code paths against “possibly unbound” locals (including keep_going handling in recursive submodule updates) and adding regression tests for the corrected behaviors.
Changes:
- Add regression coverage for
RootModule.update(keep_going=True)whenlist_itemsfails, and forCommit._iter_from_process_or_streaminvalid inputs. - Initialize sentinel locals (
sms,line_str,line,is_detached,rref, etc.) to avoid unbound-variable crashes and satisfy static analysis. - Update basedpyright baseline to remove now-unnecessary
reportPossiblyUnboundVariablesuppressions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_submodule.py | Adds regression test ensuring keep_going=True swallows a list_items failure without later unbound-local errors. |
| test/test_commit.py | Adds regression test for explicit exceptions when _iter_from_process_or_stream receives invalid sources. |
| git/repo/base.py | Tightens a few flows to avoid unbound locals (blame parsing, rebasing check) and changes daemon export helpers. |
| git/refs/log.py | Initializes line to avoid unbound-local reports in entry_at. |
| git/objects/submodule/root.py | Ensures sms is always initialized so keep_going paths don’t trip unbound locals; imports IterableList at runtime. |
| git/objects/submodule/base.py | Initializes variables (is_detached, rref) to avoid unbound locals in update/remove flows. |
| git/objects/commit.py | Raises explicit ValueError/TypeError instead of hitting unbound-local errors for invalid process/stream inputs. |
| git/index/base.py | Avoids unbound local for key; asserts working tree presence when rewriting paths. |
| .basedpyright/baseline.json | Removes baseline entries for unbound-variable warnings that are now fixed. |
Suppressed comments (1)
git/repo/base.py:943
- Avoid using
assertfor runtime validation in library code. Ifgit_diris unexpectedly missing, raise an explicit exception rather than relying on an assertion (which can be optimized away) and risking a laterTypeErrorfromosp.join.
def _set_daemon_export(self, value: object) -> None:
assert self.git_dir is not None, "Daemon export requires a Git directory"
filename = osp.join(self.git_dir, self.DAEMON_EXPORT_FILE)
fileexists = osp.exists(filename)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
<!-- agent --> Make established repository invariants explicit, initialize loop-only locals, and move assignments ahead of exception handling so basedpyright can follow the existing control flow. Regenerate the baseline to remove all 19 reportPossiblyUnboundVariable suppressions, including the two runtime fixes from the preceding commit. Validation: basedpyright --warnings; unbaselined possibly-unbound count 0; eight focused subsystem tests passed. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
git/repo/base.py:947
_set_daemon_exporthas the same empty-stringgit_dirbehavior as_get_daemon_exportand may create/removegit-daemon-export-okin the current working directory ifgit_diris "". Treat falsy values consistently as "no git_dir".
def _set_daemon_export(self, value: object) -> None:
git_dir = getattr(self, "git_dir", None)
if git_dir is None:
return
filename = osp.join(git_dir, self.DAEMON_EXPORT_FILE)
fileexists = osp.exists(filename)
git/repo/base.py:940
_get_daemon_exportnow treats an empty stringgit_diras a valid directory and will probe forgit-daemon-export-okrelative to the current working directory. This differs from the previous truthiness check and fromcurrently_rebasing_on()(which treats falsygit_diras absent) and can lead to reading the wrong path whengit_diris "".
This issue also appears on line 942 of the same file.
git_dir = getattr(self, "git_dir", None)
if git_dir is None:
return False
filename = osp.join(git_dir, self.DAEMON_EXPORT_FILE)
return osp.exists(filename)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixup #2205, which missed 2 commits when accidentally merged.